Repository navigation
fix(op): pass a storage server_error through client authentication - #1000
Conversation
Client authentication wrapped every Storage error in a new invalid_client or unauthorized_client error. RequestError maps the HTTP status from the outermost oidc.Error, so a server_error that the Storage returned for a database fault reached the client as 401 invalid_client instead of 500 server_error. I added clientAuthError in pkg/op/client.go. It returns the Storage's own oidc.Error when its type is server_error and wraps every other error in the given base error as before. The 8 client authentication wrap sites use it, in client.go, server_legacy.go (2), token_client_credentials.go, token_code.go, token_exchange.go, token_request.go and token_revocation.go. Other typed Storage errors keep the invalid_client answer, so a Storage cannot leak client existence through a different error type. No exported signature changes. pkg/op/client_auth_error_test.go covers AuthorizeClientCredentialsClient, AuthorizeClientIDSecret and a POST to the token endpoint of op.NewOpenIDProvider with basic auth for the client_credentials and authorization_code grants. A plain Storage error (wrong secret) still answers 401 invalid_client. Fixes zitadel#990 Commands I ran with go1.26.4 darwin/arm64. Main c9e2385 plus the new test file $ go test -count=1 -run 'StorageErrors' -v ./pkg/op/ --- FAIL: TestAuthorizeClientCredentialsClient_StorageErrors/typed_server_error_passes_through expected: "server_error" actual : "invalid_client" --- FAIL: TestAuthorizeClientIDSecret_StorageErrors/typed_server_error_passes_through expected: "server_error" actual : "invalid_client" --- FAIL: TestTokenEndpoint_ClientAuthStorageErrors/client_credentials/typed_server_error_passes_through expected: 500 actual : 401 --- FAIL: TestTokenEndpoint_ClientAuthStorageErrors/authorization_code/typed_server_error_passes_through expected: 500 actual : 401 (the 4 plain_error_stays_invalid_client subtests PASS) FAIL github.com/zitadel/oidc/v3/pkg/op 1.416s This branch $ go test -count=1 -run 'StorageErrors' -v ./pkg/op/ (all 8 subtests PASS) ok github.com/zitadel/oidc/v3/pkg/op 0.986s $ go test -race -count=1 ./pkg/... ok github.com/zitadel/oidc/v3/pkg/client 3.872s ok github.com/zitadel/oidc/v3/pkg/client/rp 3.915s ok github.com/zitadel/oidc/v3/pkg/client/rp/cli 1.595s ok github.com/zitadel/oidc/v3/pkg/client/rs 5.606s ok github.com/zitadel/oidc/v3/pkg/crypto 2.753s ok github.com/zitadel/oidc/v3/pkg/http 4.408s ok github.com/zitadel/oidc/v3/pkg/oidc 4.009s ok github.com/zitadel/oidc/v3/pkg/op 10.268s ok github.com/zitadel/oidc/v3/pkg/strings 4.149s (main c9e2385 gives the same 9 ok lines) $ gofmt -l pkg/op (no output) $ go vet ./pkg/op/ (no output) $ staticcheck -tags '' ./... (2026.2.1) $ staticcheck -tags no_otel ./... $ staticcheck -tags create_regression_data ./... (no findings for all three) End to end over real HTTP (httptest.NewServer, op.NewProvider with the example storage, Storage returns oidc.ErrServerError() from AuthorizeClientIDSecret and ClientCredentials) POST /oauth/token grant_type=client_credentials, basic auth main 401 {"error":"invalid_client"} this branch 500 {"error":"server_error"} The same Storage fault on authorization_code, revocation and device authorization goes from 401 or 400 on main to 500 server_error on this branch, through op.NewProvider and through op.RegisterLegacyServer. Legacy server introspection goes from 400 unauthorized_client to 500 server_error. The op.Provider introspection handler writes 401 for every error and stays as it is. A plain Storage error gives the same response on main and on this branch in every case. Not run here are CodeQL and the Go 1.27 CI job.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused classification fix is consistently applied and adequately covered by regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Preserves storage-originated server_error responses during client authentication instead of flattening them to client errors.
Changes:
- Adds centralized
clientAuthErrorclassification. - Applies it across eight authentication paths.
- Adds unit and endpoint-level regression tests.
| File | Description |
|---|---|
pkg/op/client.go |
Adds helper and updates Basic authentication. |
pkg/op/token_request.go |
Preserves server errors during secret validation. |
pkg/op/token_client_credentials.go |
Updates client-credentials authentication. |
pkg/op/token_code.go |
Updates authorization-code client lookup. |
pkg/op/token_exchange.go |
Updates token-exchange client lookup. |
pkg/op/token_revocation.go |
Updates revocation client lookup. |
pkg/op/server_legacy.go |
Updates legacy authentication paths. |
pkg/op/client_auth_error_test.go |
Adds regression and HTTP response tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wim07101993
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. Just a couple of comments.
Two more things outside the diff:
ClientJWTAuth(client.go:104) isn't converted.- Could you add a sentence to
GetClientByClientID,AuthorizeClientIDSecretandClientCredentialsinstorage.go? Something like: "returnoidc.ErrServerError()only for genuine server faults; an unknown client or wrong secret should be a plain error, and aserver_erroranswers 500". At the moment an implementer only learns this from an unexported comment.
Two small corrections to the description:
clientAuthError returned the *oidc.Error that errors.As matched inside the Storage error. That dropped an outer StatusError, so a 503 from op.NewStatusError became 500 on the LegacyServer token, revocation and introspection endpoints. It also handed the Storage's returnParent flag to MarshalJSON, which panics on a nil parent, and it cut the rest of the error chain. The helper now returns oidc.ErrServerError().WithParent(err), so the whole Storage error stays the parent. It checks for a typed nil *oidc.Error before reading ErrorType. ClientJWTAuth uses the helper too. The Storage docs of GetClientByClientID, AuthorizeClientIDSecret and ClientCredentials now say when to return oidc.ErrServerError(). TestClientAuthStorageErrors sends token, revocation and introspection requests to the Provider and to the LegacyServer through authorizePaths, and fails each client authentication lookup with a server_error, a server_error with a nil parent returned to the client, a server_error in a 503 StatusError, a plain error and a typed client error. Each of the 9 call sites has a failing row. TestClientBasicAuth and TestClientJWTAuth gain server_error, error chain, typed client error and typed nil rows, asserted with ErrorAs and the error type. The test generates the RSA key once.
|
Thanks @wim07101993, you were right on both counts in client.go, and everything you raised is in 28daa19.
`clientAuthError` no longer returns the inner error. It checks `oidcErr != nil` first, so a typed-nil `*oidc.Error` falls through to the 401 path as on main, and a server_error now comes back as a new `oidc.ErrServerError().WithParent(err)`. The Storage error stays the parent, so a `StatusError` in the chain keeps its code, nothing from the Storage error's `returnParent` leaks into the response, and its description isn't shown to the client. With `op.NewStatusError(oidc.ErrServerError().WithParent(dbErr), 503)` the LegacyServer answers 503 again on token, revocation and introspection, where the old head gave 500. `ClientJWTAuth` goes through the helper too, and `GetClientByClientID`, `AuthorizeClientIDSecret` and `ClientCredentials` in storage.go each carry the sentence you suggested.
The endpoint test is now one table over `authorizePaths`, for the Provider and the LegacyServer, covering token (client_credentials, authorization_code and token_exchange), revocation and introspection. It runs a server_error, a server_error with `WithReturnParentToClient(true)` and no parent, the 503 `StatusError`, a plain error and a typed `ErrInvalidRequest`. `NewStorage` runs once, so the RSA key is generated once. `TestClientBasicAuth` has the server_error, typed-nil and wrapped chain rows checked with `ErrorAs` plus `ErrorType`, `TestClientJWTAuth` has key storage rows, and the duplicate tests and `clientCredentialsErrorStorage` are gone. Reverting any one of the 9 call sites fails at least one subtest, and so does dropping the `ErrorType` check or the nil guard.
`go test -race ./pkg/op/...` and `go vet ./...` pass. The rest of `go test ./...` passes apart from `TestRoutes` and `TestServerRoutes`, which expect `expires_in` 299 and get 300 on this machine on unchanged main as well. One thing I left alone is the LegacyServer's client_credentials grant, which still returns the Storage error as is, as main does, so a plain wrong secret error there answers 500. I can make that path go through the helper in this PR or a follow-up, whichever you prefer.
Thank you,
Michael Lip
Founder of https://zovo.one. Chrome extensions with 14,000+ users on public Chrome Web Store pages (27 Oct 2026).
Also building https://makeroll.com and https://ml0x.com, open-source guardrails for Claude Code agents
Open source has to win, and small fixes like this one are my part of it
GitHub Sponsors keeps these fixes coming, https://github.com/sponsors/theluckystrike
|
wim07101993
left a comment
There was a problem hiding this comment.
Thanks for the update. Looks good, will merge
|
@wim07101993 I merged main into the branch again (21f8b3c), so it's no longer behind. The required "Go 1.26 test" and "Go 1.27 test" checks come from release.yml, and that run is waiting for workflow approval because this is my first PR here, so they haven't run on this PR yet. Once someone approves the run it should be ready to merge. Thank you, |
|
🎉 This PR is included in version 3.51.17 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Which Problems Are Solved
oidc.ErrServerError()(database down, corrupt row) gets a 401invalid_clientanswer instead of a 500server_error. The client sees a credentials error for a server fault and retries or rotates secrets for nothing.oidc.ErrInvalidClient().WithParent(err)puts a new error on the outside, andRequestErrormaps the status from the outermost*oidc.Erroronly, so the typedserver_erroris never seen.How the Problems Are Solved
New unexported helper
clientAuthError(base, err)inpkg/op/client.go. It returns the Storage's own error when that error is aserver_error, andbase.WithParent(err)for everything else.The 8 call sites that wrapped Storage client errors now use it. That covers
AuthorizeClientCredentialsClient,AuthorizeClientIDSecret,AuthorizeCodeClient,AuthorizeTokenExchangeClient,ParseTokenRevocationRequest,ClientBasicAuth,LegacyServer.VerifyClientand the legacy server introspection client check.Only
server_errorpasses through. A wrong secret or an unknown client still answersinvalid_client, so the response does not leak more than it does today. No exported signature changes.Additional Changes
pkg/op/client_auth_error_test.gohas unit tests forAuthorizeClientCredentialsClientandAuthorizeClientIDSecretand a token endpoint test throughop.NewOpenIDProviderwith basic auth for theclient_credentialsandauthorization_codegrants. Each runs one Storage that returnsoidc.ErrServerError()and one that returns a plain error.go test -race -count=1 ./pkg/...(the CI command) gives 9 ok packages on main and on this branch.gofmt -l pkg/opandgo vet ./pkg/op/are clean.staticcheck2026.2.1 with the tags '',no_otelandcreate_regression_datareports nothing. All with go1.26.4.httptest.NewServer, the example storage and a Storage that returnsoidc.ErrServerError()fromAuthorizeClientIDSecretandClientCredentials. Aserver_errorfromGetClientByClientIDgives the same main and branch answers for the code and revoke rows.op.Provider's introspection handler writes 401 for every error, on main and on this branch. I left it as it is.Additional Context
server_errorare flattened toinvalid_clientduring client authentication #990client_credentialsgrant) keep the behaviour from fix: oidc server error to http status mapping #865 and are unchanged.